Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added getIdentityFlags method #59

Merged
merged 4 commits into from
Sep 4, 2024
Merged

Conversation

jackforesightmobile
Copy link
Contributor

No description provided.

@matthewelwell
Copy link
Contributor

Hey @jackforesightmobile , thanks for this. I have similar feedback on this one and the one in the kotlin client.

I appreciate that we want to keep things consistent between these clients and the other SDKs, but I think this adds extra confusion at this point since getFeatureFlags and getIdentityFlags are doing very similar things (as far as I can tell).

As far as I can see, we have a few options here that might be better:

  1. Add an optional traits argument to the existing getFeatureFlags method (and raise an exception if they are provided without an identity).
  2. (Breaking change) Update the method signature to the getFeatureFlags method that takes a new Identity object which includes the identifier and (optionally) any traits.
  3. (Breaking change) Remove the identity argument from the getFeatureFlags method in favour of getIdentityFlags.

I think my preference at this point is (1), but I'm keen to know what you think from an idiomatic swift / kotlin perspective?

@jackforesightmobile
Copy link
Contributor Author

Hi @matthewelwell, I think I would agree that option 1 would be the best. I can implement that no problem and I'll update the docs to reflect the change to the method.

Copy link
Contributor

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with a minor comment.

FlagsmithClient/Classes/Flagsmith.swift Show resolved Hide resolved
@matthewelwell matthewelwell merged commit 0ae106d into Flagsmith:main Sep 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants